Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables #8711

Merged
merged 3 commits into from
Nov 22, 2018

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Aug 21, 2018

  • current sequtils.toSeq is limited: it can't be used in UFCS, and can't be used in a number of other contexts (eg iterables that are not iterators, or even when calling a proc that returns an iterator)
  • this fixes all of this

See test cases for more details but here's the skinny:

iterator myIter1():auto{.inline.}= ...
iterator myIter2():int {.closure.} = ...

doAssert [1,2].toSeq == @[1,2]
doAssert @[1,2].toSeq == @[1,2]
doAssert myIter1.toSeq == @[1,2]
doAssert toSeq(myIter1) == @[1,2]
doAssert myIter2.toSeq == @[1,2]
doAssert toSeq(myIter2) == @[1,2]

proc myIter3():auto= ... # returns iterator int
proc myIter4(a:int):auto= ... # returns iterator int
doAssert myIter3().toSeq == @[1,2]
doAssert toSeq(myIter3()) == @[1,2]

doAssert myIter4(1).toSeq == @[1,2]
doAssert toSeq(myIter4(1)) == @[1,2]

# this is the only case that can't be used with UFCS as myIter5(1) has no type:
iterator myIter5(a:int):auto = ...
doAssert = toSeq(myIter5(3)) == @[1,2]

note

  • the split between toSeq1 and toSeq2 is needed because of this bug [1]
  • toSeq handles the remaining cases (where param must be untyped), eg: toSeq(myIter5(3)) with iterator myIter5(a:int):auto = ...

fixes

links

@timotheecour timotheecour changed the title universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables [WIP] universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables Aug 21, 2018
@timotheecour timotheecour changed the title [WIP] universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables [WIP] [HELP NEEDED: 1 last thing missing] universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables Aug 25, 2018
@timotheecour timotheecour reopened this Aug 25, 2018
@timotheecour timotheecour changed the title [WIP] [HELP NEEDED: 1 last thing missing] universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables [WIP] universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables Aug 25, 2018
@timotheecour timotheecour changed the title [WIP] universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables universal toSeq: works with UFCS; works with inline & closure iterators, and with iterables Aug 25, 2018
@timotheecour
Copy link
Member Author

/cc @Araq

@timotheecour
Copy link
Member Author

/cc @Araq friendly ping

@Araq
Copy link
Member

Araq commented Sep 18, 2018

Sorry, PRs like these are delayed until finally we got 0.19 out.

@timotheecour
Copy link
Member Author

/cc @Araq ping now that 0.19 is out

@timotheecour
Copy link
Member Author

timotheecour commented Oct 24, 2018

@Araq anything holding this back? there are 7 +1's and I'd really like to have this in

@krux02
Copy link
Contributor

krux02 commented Oct 24, 2018

Generally I have to say, I like it. I think a toSeq that just works more often is very valuable. I just don't like the compiles directives. It might cause slow compilation, but I am not sure about it since I honestly think they are the best available option here. Let's see what Araq has to say. From me you get green light.

@Araq
Copy link
Member

Araq commented Oct 24, 2018

I think it's overly complicated and it will cause regressions.

@timotheecour
Copy link
Member Author

timotheecour commented Oct 24, 2018

I think it's overly complicated

not sure what you mean by that; implementation or user-facing API ?

if the latter, I have to disagree; I want a toSeq that "just works" in all contexts (or as many contexts as possible) ; and having it work in method call syntax (in all cases except 1 as mention in top msg) is also very valuable

if the former, I can try to see whether something can be simplified in implementation

it will cause regressions

impossible for me to disprove unless we implement (part of) #8638

@Araq Araq requested a review from krux02 October 25, 2018 08:44
@timotheecour
Copy link
Member Author

timotheecour commented Nov 1, 2018

@krux02 friendly ping (since @Araq assigned it to you)

@timotheecour
Copy link
Member Author

@krux02 ping

@timotheecour
Copy link
Member Author

/cc @Araq @krux02
should I rename the universal toSeq to toSeq2(or a name of your choice), leaving the original toSeq untouched, to alleviate any hypothetical concern you had about it will cause regressions so I can move forward with this PR? please...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants